[lldb][test] Fix running TestWithLimitDebugInfo.py on Windows #150579
Conversation
igorkudrin
requested review from
DavidSpickett,
adrian-prantl,
labath and
slydiman
|
@llvm/pr-subscribers-lldb Author: Igor Kudrin (igorkudrin) ChangesWhen debug info categories were set for a test method with the For example, the tests in Full diff: https://github.com/llvm/llvm-project/pull/150579.diff 2 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index a5f58373ede75..72c79520773b4 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -431,6 +431,22 @@ def impl(func):
return impl
+def remove_test_categories(remove_categories):
+ """Remove test categories from a TestCase method"""
+
+ def impl(func):
+ try:
+ if hasattr(func, "categories"):
+ updated = [c for c in func.categories if c not in remove_categories]
+ setattr(func, "categories", updated)
+ except AttributeError:
+ raise Exception("Cannot assign categories to inline tests.")
+
+ return func
+
+ return impl
+
+
def no_debug_info_test(func):
"""Decorate the item as a test what don't use any debug info. If this annotation is specified
then the test runner won't generate a separate test for each debug info format."""
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 63fadb59a82a1..48cef199737e2 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1799,6 +1799,7 @@ def no_reason(_):
for cat in categories:
@decorators.add_test_categories([cat])
+ @decorators.remove_test_categories(categories)
@wraps(attrvalue)
def test_method(self, attrvalue=attrvalue):
return attrvalue(self)
|
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Agree with the intent, very confused by the implementation.
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
This makes sense, but I wouldn't want people to start using @remove_test_categories randomly. Could you make it so that the removal logic is internal to the magic test multiplier? The code is already dealing with categories explicitly, so I think that you could just replace the @add_test_categories thingy with test_method.categories = final_set_of_categories
When debug info categories were set for a test method with the `@add_test_categories` decorator, they were all added to its "categories" attribute. If some of these categories were not supported, `LLDBTestResult.startTest()` skipped all variants of the test method. For example, the tests in `TestWithLimitDebugInfo.py` use the categories `dwarf` and `dwo`. However, since `dwo` is not supported on Windows, all the tests in this file were skipped, even though the tests for `dwarf` could be run.
igorkudrin
force-pushed
the
lldb-fix-TestWithLimitDebugInfo-on-Windows
branch
from
9979523 to
f687e98
Compare
Done, PTAL |
| test_method_categories = getattr(attrvalue, "categories", []) | ||
| dbginfo_categories = set(test_method_categories) & set( |
There was a problem hiding this comment.
How about this:
| test_method_categories = getattr(attrvalue, "categories", []) | |
| dbginfo_categories = set(test_method_categories) & set( | |
| test_method_categories = set(getattr(attrvalue, "categories", [])) | |
| all_dbginfo_categories = set( |
There was a problem hiding this comment.
With this change, the property may have different types in different cases. Would this be OK? I mean, from the design perspective, because, of course, Python is lenient enough for this to work. And I suppose that you are also suggesting replacing other_categories + [cat] with other_categories | {cat}, right?
There was a problem hiding this comment.
We should keep the type consistent, but you can do that by converting the result to a list with list(xxx) at an appropriate time. The thing that was bothering me was the combination of list- and set-based operations, which made it hard to follow.
Changing the property type to always be a set might also be good idea, but I'd do that separately.
There was a problem hiding this comment.
Thanks, I've updated the patch accordingly.
I wouldn't expect any noticeable performance gain from changing the property type, so I'm not inclined to work on it.
| if dbginfo_categories: | ||
| other_categories = [ | ||
| category | ||
| for category in test_method_categories | ||
| if category not in dbginfo_categories | ||
| ] | ||
| else: | ||
| other_categories = test_method_categories | ||
| dbginfo_categories = [ |
There was a problem hiding this comment.
| if dbginfo_categories: | |
| other_categories = [ | |
| category | |
| for category in test_method_categories | |
| if category not in dbginfo_categories | |
| ] | |
| else: | |
| other_categories = test_method_categories | |
| dbginfo_categories = [ | |
| dbginfo_categories = test_method_categories & all_dbginfo_categories | |
| other_categories = test_method_categories.difference(all_dbginfo_categories) | |
| if not dbginfo_categories: | |
| dbginfo_categories = [ |
4 participants
When debug info categories were set for a test method with the
@add_test_categoriesdecorator, they were all added to its "categories" attribute. If some of these categories were not supported,LLDBTestResult.startTest()skipped all variants of the test method.For example, the tests in
TestWithLimitDebugInfo.pyuse the categoriesdwarfanddwo. However, sincedwois not supported on Windows, all the tests in this file were skipped, even though the tests fordwarfcould be run.